Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Develop Update Submodules #515

Merged
merged 5 commits into from
Aug 29, 2023
Merged

Develop Update Submodules #515

merged 5 commits into from
Aug 29, 2023

Conversation

Michael7371
Copy link
Contributor

@Michael7371 Michael7371 commented Aug 3, 2023 •
edited
Loading

PR Details

Description

Updating submodules using this command: git submodule update --remote --merge

  • asn1_codec: Updating the commit reference from b73989 to commit 792abc0.
  • jpo-cvdp: Updating the commit reference from fa9c0ab to the commit 4d1ba2e.
  • jpo-s3-deposit: Updating the commit reference from 2880f82 to commit 4566ed1.
  • jpo-sdw-depositor: Updating the commit reference from be99dcc to commit 00c070f.
  • jpo-security-svcs: Updating the commit reference from 2c30cd2 to commit 7e6a733.

Related Issue

N/A

Motivation and Context

Includes many changes, one of which is that this commit includes the J2735 ASN file. Also noticed that many other modules were out of date.

How Has This Been Tested?

The submodules updated have already been thoroughly tested, this just updates the reference to those modules.

Types of changes

  • Defect fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that cause existing functionality to change)
  • Update submodule references

Checklist:

  • I have added any new packages to the sonar-scanner.properties file
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
    ODE Contributing Guide
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@Michael7371 Michael7371 changed the title Develop Update asn1_codec Commit Develop Update Submodules Aug 3, 2023
@paulbourelly999 paulbourelly999 requested review from paulbourelly999 and removed request for dan-du-car August 24, 2023 15:26
paulbourelly999
paulbourelly999 requested changes Aug 24, 2023
View reviewed changes
Copy link
Contributor

@paulbourelly999 paulbourelly999 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TonyEnglish @Michael7371 Discovered issue with this fork. Was able to build docker images successfully by using git clone --recurse-submodules but ran into issue running kafka container from docker compose. Issue is related to https://github.com/Michael7371/jpo-ode/blob/47d21847a2f087545d7e9ff367e70b07478c881a/docker-compose.yml#L37
https://github.com/Michael7371/jpo-ode/blob/47d21847a2f087545d7e9ff367e70b07478c881a/sample.env#L33.

Kafka container fails to start with the following docker compose error :

Error response from daemon: invalid volume specification: 'C:/var/run/docker.sock:/var/run/docker.sock:rw'
After removing ${DOCKER_SHARED_VOLUME_WINDOWS} from volume declaration in docker compose the issue goes away. There is no README instruction to edit this environment variable at all in installation process so maybe the README of the sample.env needs to be updated. What do you guys think? If this is not concerning I can approve the PR.

NOTE: Only tested docker build and docker compose up. Please let me know if you want me to test specific functionality

@Michael7371
Copy link
Contributor Author

@TonyEnglish @Michael7371 Discovered issue with this fork. Was able to build docker images successfully by using git clone --recurse-submodules but ran into issue running kafka container from docker compose. Issue is related to https://github.com/Michael7371/jpo-ode/blob/47d21847a2f087545d7e9ff367e70b07478c881a/docker-compose.yml#L37 https://github.com/Michael7371/jpo-ode/blob/47d21847a2f087545d7e9ff367e70b07478c881a/sample.env#L33.

Kafka container fails to start with the following docker compose error :

Error response from daemon: invalid volume specification: 'C:/var/run/docker.sock:/var/run/docker.sock:rw' After removing ${DOCKER_SHARED_VOLUME_WINDOWS} from volume declaration in docker compose the issue goes away. There is no README instruction to edit this environment variable at all in installation process so maybe the README of the sample.env needs to be updated. What do you guys think? If this is not concerning I can approve the PR.

NOTE: Only tested docker build and docker compose up. Please let me know if you want me to test specific functionality

Hello @paulbourelly999, nice find! I was struggling to reproduce this error until I tried to use “docker compose up -d” from my WSL Ubuntu shell directly and that seemed to produce the error. Windows PowerShell didn't seem to have an issue creating the volume with the “DOCKER_SHARED_VOLUME_WINDOWS” variable set to either the “C:” value or an empty value. Let me know if this checks out with what you are seeing.

I think for now I will change the default value for that variable in the sample.env to be empty and add some documentation to the README about this error if the behavior above is the same for you.

@Michael7371 Michael7371 mentioned this pull request Aug 24, 2023
Closed
2 tasks
@Michael7371
Copy link
Contributor Author

@paulbourelly999 I pushed those changes an also created an issue to address this fully in a seperate PR: #517

@paulbourelly999
Copy link
Contributor

changes an also created an issue to address this fully in a seperate PR: #517

That sounds perfect. For some reason the changes are not showing up here but are in the other PR. Is this an issue.?

@Michael7371
Copy link
Contributor Author

changes an also created an issue to address this fully in a seperate PR: #517

That sounds perfect. For some reason the changes are not showing up here but are in the other PR. Is this an issue.?

Yes, good check! I just pushed a commit to this branch with those changes.

paulbourelly999
paulbourelly999 previously approved these changes Aug 28, 2023
View reviewed changes
Copy link
Contributor

@paulbourelly999 paulbourelly999 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@Michael7371 Michael7371 mentioned this pull request Aug 28, 2023
Merged
10 tasks
@paulbourelly999 paulbourelly999 merged commit 4cf0908 into usdot-jpo-ode:develop Aug 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paulbourelly999 paulbourelly999 paulbourelly999 approved these changes

@SaikrishnaBairamoni SaikrishnaBairamoni Awaiting requested review from SaikrishnaBairamoni

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Michael7371 @paulbourelly999